feat: Invoked IText2SqlHook.SqlExecuting in SqlSelectFn and ExecuteQueryFn - #1392

• Call IText2SqlHook.SqlExecuting before deserializing message.FunctionArgs.
• Enable hooks to rewrite SQL payloads (e.g., inject trace tags) before execution.
• Apply the same pre-deserialization hook order to select and multi-statement query paths.

graph TD
msg["RoleDialogModel"] --> fn["SQL driver fn"] --> hook(["IText2SqlHook"]) --> deser["Deserialize args"] --> exec["SqlExecuteService"] --> db[("Database")]
hook -. "db type/conn" .-> exec
subgraph Legend
direction LR
_fn["Function"] ~~~ _hook(["Hook"]) ~~~ _db[("Database")]
end

Loading

The following are alternative approaches to this PR:

1. Hook operates on typed args (post-deserialization)

• ➕ Avoids mutating raw JSON string payloads (less brittle).
• ➕ Enables compile-time-safe rewrites (e.g., statement list vs single statement).
• ➖ Requires per-function typed hook contracts (more plumbing across functions).
• ➖ Harder to apply uniformly when functions have different arg types.

2. Add a dedicated SQL-rewrite method (string in, string out)

• ➕ Clear contract: rewrite SQL only, without mutating the whole message.
• ➕ Easier to test and reason about than rewriting FunctionArgs.
• ➖ Doesn’t naturally cover multi-field args unless expanded beyond a single string.
• ➖ May still need per-function integration for statement arrays vs single statements.

Recommendation: The PR’s approach (calling SqlExecuting(message) before deserialization) is a pragmatic way to let hooks rewrite the exact SQL payload that will be executed, and it applies uniformly across functions. If this pattern expands, consider evolving the hook API toward typed-args or explicit SQL-rewrite contracts to reduce coupling to JSON payload structure.

Enhancement (2) +10 / -2

ExecuteQueryFn.csRun SqlExecuting hook before ExecuteQueryArgs deserialization +5/-1

Run SqlExecuting hook before ExecuteQueryArgs deserialization

• Moves 'IText2SqlHook.SqlExecuting(message)' ahead of JSON deserialization so hooks can mutate 'message.FunctionArgs' (e.g., inject trace tags) before the SQL statements are read and executed. Keeps subsequent DB type and connection resolution behavior intact.

src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs

SqlSelectFn.csRun SqlExecuting hook before SqlStatement deserialization +5/-1

Run SqlExecuting hook before SqlStatement deserialization

• Invokes 'IText2SqlHook.SqlExecuting(message)' before deserializing 'SqlStatement', enabling pre-parse rewrites of the SQL/function payload. Removes the later hook acquisition to avoid duplicate resolution and ensure ordering is correct.

src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs

Description

SqlExecuting is invoked before existing error-handling paths (and SqlSelectFn has no local
handling at all), so a throwing hook can abort execution without producing the usual controlled
message.Content/StopCompletion outcomes. This creates a new unhandled-exception path introduced
by this PR.

Code

src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs[R25-28]

+        var dbHook = _services.GetRequiredService<IText2SqlHook>();
+        // The hook may rewrite message.FunctionArgs (e.g. tag the statements for
+        // traceability), so it must run before the args are deserialized.
+        await dbHook.SqlExecuting(message);

Evidence

The new hook call occurs before ExecuteQueryFn enters its try/catch (which starts later around SQL
execution), and SqlSelectFn has no try/catch at all. The repository’s HookEmitter.Emit shows the
established pattern of isolating hook exceptions with try/catch + logging.

src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs[23-80]
src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs[16-44]
src/Infrastructure/BotSharp.Core/Infrastructures/HookEmitter.cs[38-64]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`IText2SqlHook.SqlExecuting` is now awaited before the function’s existing try/catch handling (and `SqlSelectFn` has none), so any hook exception can bubble out and bypass normal failure reporting.

### Issue Context
The codebase already has a standard safe hook-dispatch mechanism (`HookEmitter.Emit`) that catches and logs hook exceptions per hook.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs[25-35]
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs[16-34]
- src/Infrastructure/BotSharp.Core/Infrastructures/HookEmitter.cs[38-64]

### Proposed fix
Wrap `SqlExecuting` invocation with error handling consistent with the rest of the system. Prefer using `HookEmitter.Emit<IText2SqlHook>(...)` (which already catches/logs hook exceptions) or add a local try/catch around `SqlExecuting` that sets `message.Content`, `message.StopCompletion`, and returns `false` in a controlled way.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

SqlExecuting is invoked on a single IText2SqlHook resolved via GetRequiredService, so other
registered/matching hooks won’t receive SqlExecuting and cannot rewrite FunctionArgs. This is
inconsistent with the repo’s hook infrastructure which supports multiple hooks per agent via
GetHooks/HookEmitter.Emit.

Code

src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs[R18-22]

+        var dbHook = _services.GetRequiredService<IText2SqlHook>();
+        // The hook may rewrite message.FunctionArgs (e.g. tag the statement for
+        // traceability), so it must run before the args are deserialized.
+        await dbHook.SqlExecuting(message);
+

Evidence

The hook system is explicitly multi-hook capable (GetHooks uses GetServices<T>()), and existing
code emits SqlGenerated through HookEmitter.Emit across all hooks. The new code path instead
calls SqlExecuting only on a single resolved hook instance, skipping any additional hooks.

src/Infrastructure/BotSharp.Abstraction/Hooks/HookProvider.cs[8-12]
src/Plugins/BotSharp.Plugin.SqlDriver/Hooks/SqlDriverPlanningHook.cs[25-28]
src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs[16-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SqlExecuting` is called on a single resolved `IText2SqlHook`, but the repo supports multiple hooks per agent. As a result, only one registered hook receives the new `SqlExecuting` event, and others cannot apply SQL rewrites/tags.

### Issue Context
- `HookProvider.GetHooks<T>` is built on `GetServices<T>()`, enabling multiple hooks.
- `SqlDriverPlanningHook` already uses `HookEmitter.Emit<IText2SqlHook>` for `SqlGenerated`.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/SqlSelectFn.cs[16-34]
- src/Plugins/BotSharp.Plugin.SqlDriver/Functions/ExecuteQueryFn.cs[25-35]
- src/Infrastructure/BotSharp.Abstraction/Hooks/HookProvider.cs[8-12]
- src/Plugins/BotSharp.Plugin.SqlDriver/Hooks/SqlDriverPlanningHook.cs[25-28]

### Proposed fix
Dispatch `SqlExecuting` through `HookEmitter.Emit<IText2SqlHook>(_services, hook => hook.SqlExecuting(message), message.CurrentAgentId)` (or iterate `_services.GetHooks<IText2SqlHook>(message.CurrentAgentId)`). Keep the single-hook resolution only for `GetDatabaseType` / `GetConnectionString` if the design requires a single authoritative provider.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools